Skip to content

fix(blog): sanitize descriptionHtml before set:html - #207

Merged
steipete merged 2 commits into
openclaw:mainfrom
SebTardif:fix/blog-dek-sanitize-html
Jul 12, 2026
Merged

fix(blog): sanitize descriptionHtml before set:html#207
steipete merged 2 commits into
openclaw:mainfrom
SebTardif:fix/blog-dek-sanitize-html

Conversation

@SebTardif

@SebTardif SebTardif commented Jul 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

Blog posts can set descriptionHtml (intentional links in the dek). That
value is rendered with Astro set:html without sanitization, so a malicious
content PR can inject script tags, event handlers, or javascript: hrefs
into the built site.

Evidence

Terminal output from the patched sanitizer and unit suite on the PR branch.

$ bun -e 'import { sanitizeHtml } from "./src/lib/sanitize-html.ts"; ...'
input: hi <script>alert(1)</script> <a href="javascript:alert(1)">x</a> <a href="https://ok.test" target="_blank">ok</a>
output: hi &lt;script&gt;alert(1)&lt;/script&gt; <a>x</a> <a href="https://ok.test" target="_blank" rel="noopener noreferrer">ok</a>
has_script_tag: false
has_javascript_href: false
has_noopener: true

$ bun test src/lib/sanitize-html.test.ts
5 pass
0 fail

Summary

  • Add src/lib/sanitize-html.ts allowlist sanitizer
  • Run blog dek through sanitizeHtml(...) before set:html
  • Tests for script strip, javascript: drop, noopener, onclick strip

Real behavior proof

  • Behavior or issue addressed: untrusted descriptionHtml could inject XSS via set:html
  • Real environment tested: macOS, bun 1.3.14, branch fix/blog-dek-sanitize-html
  • Exact steps or command run after this patch: bun -e sanitize demo; bun test sanitize-html
  • Evidence after fix: terminal output above (script escaped, javascript: removed, noopener added)
  • Observed result after fix: malicious tags/attrs cannot survive sanitizeHtml; safe https anchors kept
  • What was not tested: full astro build + browser DevTools on a deployed preview (Vercel blocked for forks)

Related

@vercel

vercel Bot commented Jul 10, 2026

Copy link
Copy Markdown

@SebTardif is attempting to deploy a commit to the Jamie's projects Team on Vercel.

A member of the Team first needs to authorize it.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels Jul 10, 2026
@clawsweeper

clawsweeper Bot commented Jul 10, 2026 •
edited
Loading

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed July 11, 2026, 8:32 PM ET / July 12, 2026, 00:32 UTC.

Summary
Removes the optional raw-HTML blog description field and its set:html sink, converts the affected post to plain text, adds a source invariant test, and updates security release notes.

Reproducibility: yes. Current main has a high-confidence source reproduction from contributor-controlled descriptionHtml frontmatter through an existing content instance to Astro set:html; the exact-head proof additionally demonstrates the corrected browser behavior.

Review metrics: 2 noteworthy metrics.

  • Raw-HTML boundary: 1 field and 1 sink removed. The patch eliminates the contributor-controlled rich-description path instead of introducing another sanitizer contract.
  • Patch surface: 5 files, +14/-4. The mitigation is narrowly scoped and includes a focused source invariant.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦀 challenger crab
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Next step before merge

  • No automated repair is needed; the exact head is ready for normal maintainer merge handling.

Security
Cleared: The diff removes the untrusted raw-HTML execution boundary and introduces no dependency, workflow, secret-handling, permission, or supply-chain surface.

Review details

Best possible solution:

Merge the exact text-only schema and escaped-rendering fix while retaining the source invariant that prevents blog descriptions from regaining a raw-HTML sink.

Do we have a high-confidence way to reproduce the issue?

Yes. Current main has a high-confidence source reproduction from contributor-controlled descriptionHtml frontmatter through an existing content instance to Astro set:html; the exact-head proof additionally demonstrates the corrected browser behavior.

Is this the best way to solve the issue?

Yes. Removing the optional raw-HTML feature at both schema and render boundaries is the narrowest maintainable solution and avoids relying on a new handwritten sanitizer.

AGENTS.md: found, but no applicable review policy affected this item.

Codex review notes: model internal, reasoning high; reviewed against 10f1d9d2760b.

Label changes

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. Maintainer-provided exact-head evidence includes a production Astro build and real preview-browser check showing hostile markup rendered literally with no injected element or side effect.
  • add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦀 challenger crab and patch quality is 🦞 diamond lobster.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): Maintainer-provided exact-head evidence includes a production Astro build and real preview-browser check showing hostile markup rendered literally with no injected element or side effect.
  • remove rating: 🦪 silver shellfish: Current PR rating is rating: 🦞 diamond lobster, so this older rating label is no longer current.
  • remove status: 📣 needs proof: Current PR status label is status: 👀 ready for maintainer look.

Label justifications:

  • P2: This fixes a bounded contributor-content XSS path on the public website without affecting the core runtime.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦀 challenger crab and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): Maintainer-provided exact-head evidence includes a production Astro build and real preview-browser check showing hostile markup rendered literally with no injected element or side effect.
  • proof: sufficient: Contributor real behavior proof is sufficient. Maintainer-provided exact-head evidence includes a production Astro build and real preview-browser check showing hostile markup rendered literally with no injected element or side effect.
Evidence reviewed

What I checked:

  • Current-main vulnerability path: Current main accepts optional descriptionHtml, an existing blog post supplies it, and the article renderer passes the resulting value directly to Astro set:html. (src/pages/blog/[...slug, 10f1d9d2760b)
  • Exact-head mitigation: The final head removes descriptionHtml from the content schema and renders the ordinary description using Astro's escaped text interpolation. (src/pages/blog/[...slug, 456fc622d130)
  • Regression invariant: The added test requires escaped blog description interpolation and rejects set:html, the removed schema field, and any remaining blog frontmatter use. (tests/static-assets.test.ts:37, 456fc622d130)
  • Exact-head real behavior proof: The maintainer proof reports a 31-page production build and real Astro preview browser exercise where adversarial markup displayed literally, no injected image or link appeared, no side effect occurred, and all 38 Bun tests passed. (456fc622d130)
  • Checks and merge result: All repository-owned exact-head checks completed successfully, and the synthetic merge contains only the expected five-file patch with no whitespace errors; Vercel contexts did not execute because fork authorization was unavailable. (db4589dbc865)
  • Feature provenance: Git history attributes the original blog schema and raw-HTML article renderer to Vincent Koc's July 6 blog implementation, while the final mitigation was authored in commit 86af861. (src/pages/blog/[...slug, ec8082c0dd90)

Likely related people:

  • vincentkoc: Vincent Koc introduced the current blog schema and article renderer and has the largest recent contribution history across the central blog files. (role: introduced behavior and primary recent area contributor; confidence: high; commits: ec8082c0dd90, a5e75be, bd9fa89; files: src/pages/blog/[...slug].astro, src/content.config.ts)
  • steipete: Peter Steinberger authored the final mitigation commits, force-pushed the exact head, and supplied exact-head build, browser, test, and CI evidence. (role: final implementation author and proof provider; confidence: high; commits: 86af86108eb4, 456fc622d130; files: src/pages/blog/[...slug].astro, src/content.config.ts, tests/static-assets.test.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (1 earlier review cycle)
  • reviewed 2026-07-12T00:21:19.906Z sha 456fc62 :: needs real behavior proof before merge. :: none

@SebTardif

SebTardif commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Added What Problem This Solves, Evidence, and Real behavior proof with bun sanitize demo + unit suite output.

@clawsweeper

clawsweeper Bot commented Jul 10, 2026 •
edited
Loading

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jul 10, 2026
steipete and others added 2 commits July 11, 2026 17:13
@steipete
steipete force-pushed the fix/blog-dek-sanitize-html branch from 83aca6a to 456fc62 Compare July 12, 2026 00:16
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels Jul 12, 2026
@steipete

steipete commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Maintainer proof for final exact head 456fc622d1307860ea9fe22a004cd3e0529eb3ec:

  • Root cause: optional descriptionHtml frontmatter flowed directly into Astro set:html, allowing active markup from a content contribution to reach the built page.
  • Fix design: remove the raw-HTML feature instead of maintaining a handwritten regex sanitizer. The schema no longer accepts descriptionHtml, the sole content use is removed, and the article dek uses Astro's escaped text interpolation.
  • Regression coverage: all 38 Bun tests passed, including a source-wide assertion that the schema, blog content, and article renderer contain no descriptionHtml or dek raw-HTML sink. bun run test:built-assets passed after a 31-page production build.
  • Exact browser behavior: a unique adversarial description was built and served through the real Astro preview, then exercised in the existing Chrome profile through Peekaboo. The markup displayed literally; Chrome console checks returned [true, true, true, true]: no injected image, no injected link, no side effect, sentinel text present.
  • Independent review: structured Codex autoreview reports no accepted/actionable findings (patch is correct, confidence 0.96).
  • Hosted CI: exact-head runs 29173470332 and 29173470282 passed every repository-owned Linux, Windows, installer, unit, smoke, and shellcheck job. The macOS conditional job is intentionally skipped.
  • Preview note: both Vercel contexts refused deployment because fork authorization is required; they did not execute and are not code failures. The production build plus existing-profile browser proof exercises the exact changed rendering boundary without deployment mutation.
  • Changelog: maintainer entries credit @SebTardif for this fix and restore the entry required for the already-landed JSON-LD hardening.
  • Public Model Identifier Gate: PASS; the final candidate diff, fixtures, generated output, CI/proof text, and browser evidence introduce no model-bearing identifier.

Risk: low after proof. Files: 5, +14/-4. Sebastien remains credited as PR author and commit co-author.

Merge recommendation: supported.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 12, 2026 •
edited
Loading

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 12, 2026
@steipete
steipete merged commit 3cece56 into openclaw:main Jul 12, 2026
16 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@SebTardif @steipete